Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds extensive unit tests across multiple packages (constants, logger, metrics, policyxds, storage, utils) without modifying production code; introduces many new test functions, mocks, and helpers to increase coverage and exercise server, storage, metrics, and API key logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/utils/api_key_test.go`:
- Around line 1829-1872: The test shows ListAPIKeys panics when
ListAPIKeyParams.User is nil because filterAPIKeysByUser dereferences the user;
update the ListAPIKeys function to validate params.User (or return a clear
error) before calling filterAPIKeysByUser, and modify filterAPIKeysByUser to
defensively handle a nil user (return an error instead of panicking); add/adjust
unit tests to assert an error is returned for a nil User rather than relying on
defer/recover.
- Around line 1639-1681: The test "regenerate - database conflict with retry"
currently never triggers a conflict because mockDB.returnConflict is false and
callCount is unused; update the mock behavior so mockDB.UpdateAPIKey returns a
conflict on the first call and succeeds on the second (use callCount to track
invocations), e.g., initialize callCount := 0, set mockDB.returnConflict = true
or mockDB.updateError to a conflict on first call, then in the mock UpdateAPIKey
implementation increment callCount and clear the conflict (set
returnConflict=false or updateError=nil) after the first failure to simulate
retry; keep the test parameters and assertions the same and ensure you reference
NewAPIKeyService and service.RegenerateAPIKey (APIKeyRegenerationParams) to
validate the retry path.
- Around line 1150-1186: The test TestCreateAPIKey_ConfigStoreRollback currently
guards assertions with "if err == nil { ... }" which masks failures; update the
test to assert the expected outcome directly by replacing the conditional with
explicit assertions: call assert.NoError(t, err) (or assert.Error if the
intended behavior is failure) immediately after invoking
service.CreateAPIKey(params), then assert.NotNil(t, result) and any
rollback-related expectations; reference the test function
TestCreateAPIKey_ConfigStoreRollback and the CreateAPIKey call on the service
along with APIKeyCreationParams to locate and update the assertions.
- Around line 1801-1825: The test currently discards the result of
service.ListAPIKeys (line with `_ = err`) so it never asserts behavior; change
it to capture the returned values from ListAPIKeys and add assertions: call res,
err := service.ListAPIKeys(params) and then assert either err != nil (verify it
indicates the database failure from mockDB.getError or at least non-nil) OR if
err == nil assert that res is an empty slice (len(res) == 0). Update the test
named "list fails when both memory and database fail" to use these checks
against the ListAPIKeys return values so the test can fail on unexpected
behavior.
🧹 Nitpick comments (4)
gateway/gateway-controller/pkg/logger/logger_test.go (1)
162-198: Coverage-only test with no output assertions.The test acknowledges it can't capture
os.Stdoutoutput, so it only exercises code paths without verifying correctness of theReplaceAttrcallback. This is fine for coverage, but consider injecting abytes.Buffer(similar toTestXDSLogger) by refactoringNewLoggerto accept anio.Writer, which would allow asserting formatted source paths. This would make the test actually validate behavior.gateway/gateway-controller/pkg/metrics/metrics_test.go (2)
292-313:TestServer_Startsilently swallows errors — test can never fail.Both
Start()(line 306) andStop()(line 312) errors are ignored. With port 0,Startshould reliably succeed, so consider assertingassert.NoErroron it. Also,Stoperror on line 312 is discarded.Suggested fix
err := server.Start() - if err != nil { - t.Logf("Start returned error (may be acceptable): %v", err) - } + if err == nil { + // Clean up only if Start succeeded + ctx := context.Background() + stopErr := server.Stop(ctx) + if stopErr != nil { + t.Logf("Stop returned error: %v", stopErr) + } + } else { + t.Logf("Start returned error (may be acceptable): %v", err) + } - - // Clean up - ctx := context.Background() - server.Stop(ctx)
269-290:TestStartMemoryMetricsUpdaterrelies ontime.Sleepfor synchronization.The test sleeps 250ms to let the goroutine run, then 50ms for cleanup. While this is unlikely to be flaky in practice (the goroutine responds to context cancellation), note that these tests won't detect panics inside the goroutine since they'd crash a different goroutine than the test's.
gateway/gateway-controller/pkg/storage/sqlite_test.go (1)
36-40: Package-level mutable counters are not safe for parallel test execution.
configCounter,llmTemplateCounter, andapiKeyCounterare incremented bycreateTest*helpers without synchronization. Currently safe since no test usest.Parallel(), but if parallelism is ever added, these will race. Consider usingatomic.AddInt32or passing a*testing.T-scoped counter.
| func TestListAPIKeys_FilterError(t *testing.T) { | ||
| logger := slog.New(slog.NewTextHandler(io.Discard, nil)) | ||
|
|
||
| t.Run("list with nil user causes panic (code should validate user first)", func(t *testing.T) { | ||
| store := storage.NewConfigStore() | ||
|
|
||
| testConfig := createTestAPIConfig("api-1", "test-api") | ||
| _ = store.Add(testConfig) | ||
|
|
||
| // Add a key so we get past the memory store | ||
| key := &models.APIKey{ | ||
| ID: "key-1", | ||
| Name: "test-key", | ||
| APIKey: "hashed-key", | ||
| APIId: "api-1", | ||
| Status: models.APIKeyStatusActive, | ||
| CreatedBy: "user1", | ||
| } | ||
| _ = store.StoreAPIKey(key) | ||
|
|
||
| service := NewAPIKeyService(store, nil, nil, &config.APIKeyConfig{ | ||
| APIKeysPerUserPerAPI: 10, | ||
| Algorithm: constants.HashingAlgorithmSHA256, | ||
| }) | ||
|
|
||
| params := ListAPIKeyParams{ | ||
| Handle: "test-api", | ||
| User: nil, // This will cause a panic in filterAPIKeysByUser when logging | ||
| CorrelationID: "corr-123", | ||
| Logger: logger, | ||
| } | ||
|
|
||
| // The code has a bug: it panics when user is nil | ||
| // This test documents that behavior (ideally the code should be fixed to validate user first) | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Expected: panic due to nil user access after filterAPIKeysByUser returns error | ||
| assert.NotNil(t, r) | ||
| } | ||
| }() | ||
|
|
||
| _, _ = service.ListAPIKeys(params) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Test documents a nil-user panic bug — consider filing an issue to fix the root cause.
The test explicitly documents that ListAPIKeys panics when User is nil (line 1861), catching it with defer/recover. While documenting known defects in tests is reasonable, the production code should ideally validate User before dereferencing. If this is a known issue, consider opening an issue to track the fix.
Do you want me to open a new issue to track the nil-user validation fix in ListAPIKeys?
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/utils/api_key_test.go` around lines 1829 -
1872, The test shows ListAPIKeys panics when ListAPIKeyParams.User is nil
because filterAPIKeysByUser dereferences the user; update the ListAPIKeys
function to validate params.User (or return a clear error) before calling
filterAPIKeysByUser, and modify filterAPIKeysByUser to defensively handle a nil
user (return an error instead of panicking); add/adjust unit tests to assert an
error is returned for a nil User rather than relying on defer/recover.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/utils/api_key_test.go`:
- Around line 2195-2209: Rename the misnamed sub-test to match its assertion and
behavior: change the t.Run name string in the test that constructs store :=
storage.NewConfigStore() and service := NewAPIKeyService(...) so it no longer
says "no db and memory store error returns error" and instead reads something
like "no db falls back to memory count of zero and passes" (also update the
inline comment near the enforceAPIKeyLimit invocation to reflect that memory
returns 0 and the test expects no error).
- Around line 1150-1184: The test TestCreateAPIKey_ConfigStoreRollback is
misnamed because it creates a normal storage.NewConfigStore and never triggers
ConfigStore.StoreAPIKey failure; either rename the sub-test to reflect a
successful creation or modify the setup to provoke a real ConfigStore error so
rollback is exercised — for example replace storage.NewConfigStore with a mock
that implements StoreAPIKey and returns an error, or adjust APIKeyCreationParams
to hit validation that causes ConfigStore.StoreAPIKey to fail, then assert that
CreateAPIKey on the APIKeyService rolls back the DB by checking mockDB
interactions; update the sub-test name and assertions accordingly to match the
chosen approach.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/api_key_test.go (1)
958-967:UpdateAPIKeymock lacks aconflictOnceOnUpdatemechanism (unlikeSaveAPIKey).
SaveAPIKeyhasconflictOnceOnSaveto simulate a conflict that resolves on retry, butUpdateAPIKeyhas no equivalent. This is why the "regenerate - database conflict with retry" test at Line 1637 can't actually exercise the retry path. Adding a parallelconflictOnceOnUpdatefield would enable a proper retry test.Suggested addition
type mockStorage struct { ... conflictOnceOnSave bool saveCallCount int + conflictOnceOnUpdate bool + updateCallCount int } func (m *mockStorage) UpdateAPIKey(apiKey *models.APIKey) error { + m.updateCallCount++ + if m.conflictOnceOnUpdate && m.updateCallCount == 1 { + return storage.ErrConflict + } if m.returnConflict { return storage.ErrConflict } if m.updateError != nil { return m.updateError } m.apiKeys[apiKey.ID] = apiKey return nil }
1 participant
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit